Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix overflowing notification buttons #3939

Merged
merged 1 commit into from
Sep 13, 2016
Merged

Conversation

ayumi
Copy link
Contributor

@ayumi ayumi commented Sep 12, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Notification options have float: right. I moved them to precede other content in the DOM, so now the other content flows around it rather than "pushing" it away.

Fix #3920

Auditors: @luixxiul @jkup

Test Plan:

  1. Open Brave and trigger a notification e.g. visit https://www.browserleaks.com/geo
  2. Horizontally resize window so the notification text overflows
  3. Confirm that the buttons don't overflow out of the notification

Should now look like:
screen shot 2016-09-12 at 16 34 09
screen shot 2016-09-12 at 16 29 57

Fix #3920

Auditors: @luixxiul @jkup

Test Plan:

1. Open Brave and trigger a notification e.g. visit https://www.browserleaks.com/geo
2. Horizontally resize window so the notification text overflows
3. Confirm that the buttons don't overflow out of the notification
@jkup
Copy link
Contributor

jkup commented Sep 13, 2016

Code LGTM! The only thought is it might be better to keep the order the same but just make sure the buttons extend the yellow notification background? My thinking is just that since the message is top left I expect it to be above the top right content on collapse.

@ayumi
Copy link
Contributor Author

ayumi commented Sep 13, 2016

@jkup I agree that'd be good, but I can't figure out the CSS to make it look like that. :/

Because this fixes an unsightly bug I think it'd be nice to merge as is.
Afterwards we should improve the styling.

@ayumi ayumi merged commit 46f87a6 into master Sep 13, 2016
@ayumi ayumi deleted the fix/notifications-overflow branch September 13, 2016 03:10
@luixxiul
Copy link
Contributor

luixxiul commented Sep 13, 2016

We need margin-left to avoid this:

screenshot 2016-09-13 14 44 24

I'm pushing a PR to fix it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants